feat(spp_change_request_v2): improve CR creation wizard UX#29
feat(spp_change_request_v2): improve CR creation wizard UX#29gonzalesedwin1123 merged 5 commits into19.0from
Conversation
- Add debounced search field with custom OWL widget for live registrant search - Search by name and registrant IDs (reg_ids.value) - Render search results as clickable HTML table rows - Show max 2 IDs per row with +N badge and tooltip for overflow - Add pagination (10 per page) with Previous/Next navigation - Display selected registrant info with all IDs as badges - Preserve search state when changing registrant selection - Add display_name to detail base using CR reference - Replace Submit for Approval with Proceed button on detail forms - Proceed validates proposed changes before navigating to main CR form - Remove New Request menu item (use list view button instead) - Remove View Registrant stat button, rename Open Form to View Registrant - Show Preview Changes stat button only in developer mode
Summary of ChangesHello @emjay0921, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly overhauls the user experience for creating Change Requests, particularly focusing on the initial registrant selection process. The changes introduce a more dynamic and user-friendly search mechanism, providing live feedback and better information display. Additionally, the navigation between the wizard, detail forms, and the main Change Request form has been refined to ensure a smoother and more intuitive user journey, reducing friction and improving overall efficiency for users initiating change requests. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the user experience of the Change Request creation wizard by implementing a debounced, paginated search for registrants. The changes are well-executed, introducing new OWL components for the search functionality and updating the backend logic accordingly. The overall implementation is robust and enhances usability. My review includes a few suggestions to further improve performance, maintainability, and conciseness of the code.
| for rec in self: | ||
| if rec.reg_ids: | ||
| parts = [] | ||
| for rid in rec.reg_ids: | ||
| if rid.value: | ||
| label = rid.id_type_as_str or "ID" | ||
| parts.append(f"{label} ({rid.value})") | ||
| rec.reg_id_display = ", ".join(parts) if parts else "" | ||
| else: | ||
| rec.reg_id_display = "" |
There was a problem hiding this comment.
The logic in this method can be made more concise and Pythonic by using a list comprehension. This would eliminate the need for the outer if/else block and the conditional expression within the join call, making the code easier to read and maintain.
parts = [
f"{(rid.id_type_as_str or 'ID')} ({rid.value})"
for rid in rec.reg_ids
if rid.value
]
rec.reg_id_display = ", ".join(parts)| setup() { | ||
| this.containerRef = useRef("container"); | ||
| onMounted(() => this._attachClickHandler()); | ||
| onPatched(() => this._attachClickHandler()); | ||
| } | ||
|
|
||
| get htmlContent() { | ||
| return this.props.record.data[this.props.name] || ""; | ||
| } | ||
|
|
||
| _attachClickHandler() { | ||
| const el = this.containerRef.el; | ||
| if (!el) return; | ||
| // Row selection | ||
| el.querySelectorAll(".o_cr_search_result").forEach((row) => { | ||
| row.onclick = (ev) => { | ||
| ev.preventDefault(); | ||
| ev.stopPropagation(); | ||
| const partnerId = parseInt(row.dataset.partnerId); | ||
| if (partnerId) { | ||
| this.props.record.update({_selected_partner_id: partnerId}); | ||
| } | ||
| }; | ||
| }); | ||
| // Pagination | ||
| el.querySelectorAll(".o_cr_page_prev, .o_cr_page_next").forEach((link) => { | ||
| link.onclick = (ev) => { | ||
| ev.preventDefault(); | ||
| ev.stopPropagation(); | ||
| const page = parseInt(link.dataset.page); | ||
| if (!isNaN(page) && page >= 0) { | ||
| this.props.record.update({_search_page: page}); | ||
| } | ||
| }; | ||
| }); | ||
| } |
There was a problem hiding this comment.
For better performance and maintainability, consider using event delegation for handling clicks instead of attaching individual onclick handlers to each result row and pagination link. By adding a single event listener to the container, you can avoid re-attaching handlers on every render (onPatched) and reduce the total number of event listeners, which is more efficient, especially with many search results.
Also, it's a good practice to always provide the radix parameter to parseInt to avoid unexpected behavior.
setup() {
this.containerRef = useRef("container");
onMounted(() => {
this.containerRef.el.addEventListener("click", this._onClick.bind(this));
});
}
get htmlContent() {
return this.props.record.data[this.props.name] || "";
}
_onClick(ev) {
// Row selection
const row = ev.target.closest(".o_cr_search_result");
if (row) {
ev.preventDefault();
ev.stopPropagation();
const partnerId = parseInt(row.dataset.partnerId, 10);
if (partnerId) {
this.props.record.update({_selected_partner_id: partnerId});
}
return;
}
// Pagination
const pageLink = ev.target.closest(".o_cr_page_prev, .o_cr_page_next");
if (pageLink) {
ev.preventDefault();
ev.stopPropagation();
const page = parseInt(pageLink.dataset.page, 10);
if (!isNaN(page) && page >= 0) {
this.props.record.update({_search_page: page});
}
}
}| <templates xml:space="preserve"> | ||
|
|
||
| <t t-name="spp_change_request_v2.CrSearchResultsField"> | ||
| <div t-ref="container" class="o_field_cr_search_results" style="width:100%" t-out="htmlContent"/> |
There was a problem hiding this comment.
The inline style style="width:100%" is redundant, as the o_field_cr_search_results class already has its width set to 100% !important in the associated CSS file (cr_search_results.css). It's best practice to rely on the stylesheet for styling to improve maintainability and separation of concerns. Please remove the inline style.
| <div t-ref="container" class="o_field_cr_search_results" style="width:100%" t-out="htmlContent"/> | |
| <div t-ref="container" class="o_field_cr_search_results" t-out="htmlContent"/> |
| def _render_search_results(self): | ||
| """Search and render paginated HTML results.""" | ||
| search_domain = self._get_search_domain() | ||
| total = self.env["res.partner"].search_count(search_domain) | ||
|
|
||
| if not total: | ||
| self.search_results_html = Markup( | ||
| "<p class='text-muted'>No registrants found.</p>" | ||
| ) | ||
| return | ||
|
|
||
| page = self._search_page or 0 | ||
| page_size = self._SEARCH_PAGE_SIZE | ||
| max_page = (total - 1) // page_size | ||
| page = min(page, max_page) | ||
|
|
||
| offset = page * page_size | ||
| partners = self.env["res.partner"].search( | ||
| search_domain, limit=page_size, offset=offset | ||
| ) | ||
|
|
||
| rows = [] | ||
| for p in partners: | ||
| # Build ALL IDs in "TypeName (value)" format, show max 2 | ||
| id_parts = [] | ||
| if p.reg_ids: | ||
| for rid in p.reg_ids: | ||
| if rid.value: | ||
| label = rid.id_type_as_str or "ID" | ||
| id_parts.append(f"{label} ({rid.value})") | ||
| if not id_parts: | ||
| id_html = Markup("") | ||
| id_title = "" | ||
| elif len(id_parts) <= 2: | ||
| id_html = escape(", ".join(id_parts)) | ||
| id_title = "" | ||
| else: | ||
| visible = escape(", ".join(id_parts[:2])) | ||
| extra = len(id_parts) - 2 | ||
| id_html = Markup( | ||
| '{} <span class="badge text-bg-secondary ms-1">' | ||
| "+{} <i class='fa fa-info-circle'></i></span>" | ||
| ).format(visible, extra) | ||
| id_title = ", ".join(id_parts) | ||
| ptype = ( | ||
| '<i class="fa fa-users"></i> Group' | ||
| if p.is_group | ||
| else '<i class="fa fa-user"></i> Individual' | ||
| ) | ||
| rows.append( | ||
| Markup( | ||
| '<tr class="o_cr_search_result" style="cursor:pointer"' | ||
| ' data-partner-id="{}" data-partner-name="{}">' | ||
| "<td>{}</td>" | ||
| '<td title="{}">{}</td>' | ||
| "<td>{}</td></tr>" | ||
| ).format( | ||
| p.id, | ||
| escape(p.name or ""), | ||
| escape(p.name or ""), | ||
| escape(id_title), | ||
| id_html, | ||
| Markup(ptype), | ||
| ) | ||
| ) | ||
|
|
||
| table = Markup( | ||
| '<table class="table table-hover table-sm mb-0 w-100">' | ||
| "<thead><tr><th>Name</th><th>ID</th><th>Type</th></tr></thead>" | ||
| "<tbody>{}</tbody></table>" | ||
| ).format(Markup("").join(rows)) | ||
|
|
||
| # Pagination header | ||
| start = offset + 1 | ||
| end = min(offset + page_size, total) | ||
| prev_cls = "text-muted" if page == 0 else "o_cr_page_prev" | ||
| next_cls = "text-muted" if page >= max_page else "o_cr_page_next" | ||
| pagination = Markup( | ||
| '<div class="d-flex justify-content-between align-items-center mb-2 px-1">' | ||
| '<small class="text-muted">{}-{} of {}</small>' | ||
| "<div>" | ||
| '<a class="{} me-3" style="cursor:pointer" data-page="{}">← Previous</a>' | ||
| '<a class="{}" style="cursor:pointer" data-page="{}">Next →</a>' | ||
| "</div></div>" | ||
| ).format(start, end, total, prev_cls, page - 1, next_cls, page + 1) | ||
|
|
||
| self.search_results_html = pagination + table |
There was a problem hiding this comment.
This method manually constructs a significant amount of HTML using string formatting. While it's done securely with Markup and escape, this approach can be difficult to read and maintain.
For better separation of concerns, consider refactoring this to use a server-side QWeb template. You could define the template in an XML file and render it using self.env['ir.qweb']._render(). This would make the Python code cleaner and the HTML structure more manageable.
…xact match for ID search - Remove ID column from search results table (show Name and Type only) - Remove ID badges from selected registrant info card - Change reg_ids.value search from ilike (partial) to = (exact match)
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 19.0 #29 +/- ##
===========================================
- Coverage 76.43% 62.97% -13.46%
===========================================
Files 33 239 +206
Lines 3093 19210 +16117
===========================================
+ Hits 2364 12098 +9734
- Misses 729 7112 +6383
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| def _onchange_request_type(self): | ||
| """Clear registrant if it doesn't match the new target type.""" | ||
| """Clear registrant and search if type changes.""" | ||
| self.search_text = False |
There was a problem hiding this comment.
Redundant type-matching conditional after unconditional search clear
Low Severity
The newly added self.search_text = False at the top of _onchange_request_type triggers a cascading _onchange_search_text call, which unconditionally clears registrant_id. This makes the subsequent conditional check (lines 380–384 that only clear registrant_id when the target type mismatches) dead code — registrant_id will always be cleared by the cascading onchange regardless of whether the type matches. The conditional misleadingly suggests selective clearing still happens.
Kept PR design choices: preview button with debug group restriction, custom search widget UX, action_view_registrant, no New Request menu. Added is_conflict_detection_enabled check on recheck button.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| component: SearchDelayField, | ||
| displayName: _t("Search with Delay"), | ||
| supportedTypes: ["char"], | ||
| extractProps: ({placeholder}) => ({placeholder}), |
There was a problem hiding this comment.
Placeholder prop never extracted from view attrs
Low Severity
extractProps: ({placeholder}) => ({placeholder}) destructures placeholder directly from the top-level fieldInfo argument, which has the shape {attrs, field, ...}. In Odoo 16/17, XML view attributes like placeholder="Enter name or ID number..." are stored at attrs.placeholder, not at the top level. The other widget in this same codebase (cel_editor_field.js) correctly uses ({attrs, options}) => ({placeholder: attrs.placeholder}). As written, placeholder is always undefined, so the search input shows no hint text.
| def _onchange_request_type(self): | ||
| """Clear registrant if it doesn't match the new target type.""" | ||
| """Clear registrant and search if type changes.""" | ||
| self.search_text = False |
There was a problem hiding this comment.
Stale search results shown after type change
Medium Severity
_onchange_request_type clears search_text but does not clear search_results_html. When the user changes the request type to one incompatible with the currently selected registrant, registrant_id is cleared. The view condition invisible="not search_results_html or registrant_id" then reveals the stale results (since registrant_id is now False), displaying rows from the old type's search domain. Clicking one of these stale rows sets registrant_id to a registrant that fails the type check in action_create_draft.
2 participants
Why is this change needed?
The CR creation wizard needed a better registrant search experience — searching by ID, live results, pagination, and improved UX flow from wizard → detail form → main CR form.
How was the change implemented?
SearchDelayFieldtriggers search after 500ms typing delaynameandreg_ids.valuefieldsCrSearchResultsFieldOWL widget renders results as a table with click-to-select via_selected_partner_idbridge field+Nbadge and native tooltip for overflow_compute_display_nameon detail base shows CR reference instead ofmodel,idMirror of: https://github.com/OpenSPP/openspp-modules-v2/pull/294
New unit tests
Unit tests executed by the author
How to test manually
Related links
https://github.com/OpenSPP/openspp-modules-v2/pull/294
Note
Medium Risk
Touches wizard search behavior and navigation flows (JS widgets + server-side onchanges + view changes), so regressions could block CR creation or selection. No auth/data model migrations, but HTML rendering (
sanitize=False) and click-handling merit careful review.Overview
Improves the CR creation wizard by replacing the
registrant_idpicker with a debouncedsearch_textfield that renders paginated, clickable HTML results; selection and pagination are bridged via_selected_partner_id/_search_pageonchanges, plus aChange Registrantaction to clear selection and re-run the last search.Adds new OWL field widgets (
search_delay,cr_search_results) with supporting XML/CSS assets, and updates detail views to use a newaction_proceed_to_crflow that blocks proceeding unless the CR has proposed changes; detail records also display the parent CR name as their display name.Tightens/cleans up CR UI: hides
Preview Changesbehindbase.group_no_one, removes the redundant “New Request” menu entry, renames the registrant open button toaction_view_registrant, and only shows “Check for Updates” when conflict detection is enabled for the CR type (via a new related flag onspp.change.request).Written by Cursor Bugbot for commit c350022. This will update automatically on new commits. Configure here.